A small part of me thinks that we should change all the references of properties to args to bring it inline with other WP core naming, now that we've broken the direct dependency to add a validator. 🤷

I could add it in this PR but i didnt want to obfuscate the discussion on #53

We have now ability_class which isn't strictly a property of the object, so I don't mind changing to args at this point. If you want to take care of it, let's put it into a separate PR to keep the current refactor lean.

+1 to changing this to use $args.

An alternative approach is to replace the validate_*() pattern with the prepare_*(): array|WP_Error() , and then throw in the constructor.

This would improve the DX both mapping and validation in the same step, but I'm not sure how flexible we want things to be at this initial stage vs once we've given the initial API + round of feedback time to percolate, so I went with the approach in the diff.

e.g. (pseudocode)

$properties = $this->prepare_args( $args );

if ( is_wp_error( $properties ) ) {
  throw \InvalidArgumentException( $properties->getMessage() );
}

// This is still outside the function so extenders don't need to reimplement it.
foreach( $properties as $name => $value ) {
  ...
}

@gziolo / @felixarntz would love some specific thoughts on this if you have them. The more I think about it the more I feel like protected function prepare_properties( array<string,mixed> ): array<validated-shape>|WP_Error is the better approach 🤔

Ok, so you prefer to have prepare_properties that returns WP_Error as soon as something goes wrong, and it gets translated to an exception, or the properties returned gets passed to the loop. That sounds good to me, to avoid having a method that only throws an exception when someting goes wrong. In fact, it might be simpler to ever introduce a filter if folks want to change this properties as an alternative to what I proposed here:

• Propose filters to use with the registered ability #37

The filter used with block types register_block_type_args comes to my find as a good reference:

https://github.com/WordPress/wordpress-develop/blob/9b6c234bc6a78ce4a57929ea5413d0b08a3d706e/src/wp-includes/class-wp-block-type.php#L565-L573

My thoughts exactly. After the chat, I'll push a change with that approach.

I'm not strongly opposed, but I also don't see the benefit of using prepare_properties and returning a WP_Error from it if something is wrong. Can you clarify why that's better?

To me it seems unnecessarily complex to rely on a method that can return WP_Error, only to turn that into an InvalidArgumentException, only to catch that and turn it into a _doing_it_wrong().

Why not simply throw the exceptions?

Clarifying that my focus is on returning the array of (prepared) args instead of void. 😅

@@felixarntz I agree with you from a code quality POV

From an implementer POV my thought was less "exceptions should be exceptional" and more "let's do it the WP™️ way 💪" :

• most the other parts of the exposed API return WP_Errors to be handled instead of throwing.
• In general WordPress developers are more comfortable returning error objects than throwing.
• most importantly: it gives me a justification to leave the return type off the method signature (since php7.4 doesn't support union return types) since I didn't want to argue about why a legacy project wanting to migrate to this API but keep their existing DTO object isn't a justifiable use case for polymorphic tech debt at this early stage. 😅

(I drafted this before you both aligned yesterday on #53 and only saw after I pushed, so in my head this was still very much a proposal ).

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw

See #54 (comment).

The only changes here are that they're now InvalidArgumentExceptions() instead of _doing_it_wrong(). The conditionals and error message are identical to what was in WP_Abilities_API::register()

I double-checked with the implementation @felixarntz used in the demo plugin and it seems to be compatible even with the strict checks for madatory properties: label, description and execute callback.

https://github.com/felixarntz/wp-ai-sdk-chatbot-demo/blob/326266fd62fc805ceac0dfd4fe71cd3cc3e7cad3/includes/Abilities/Abstract_Ability.php#L32-L48